feat: integrate Slurm plan and runtime state - #889
Conversation
Validate plan-aware shards, readiness, attempts, and finalization through a reusable immutable context. Add deterministic goldens and negative coverage for mismatch and failure paths. Closes #880 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Require scheduler-bound plan validation to start only after an attempt leaves the created state. Cover created attempts that already carry scheduler metadata and verify the integration validator imports from an isolated Slurm wheel. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
| import json | ||
| import posixpath | ||
| from dataclasses import dataclass | ||
| from datetime import UTC, datetime, timedelta |
There was a problem hiding this comment.
datetime.UTC is only available from Python 3.11, but data-designer-slurm declares Python 3.10 support and the Slurm CI job currently runs only 3.11. On 3.10 this file fails during collection, so none of the new integration checks run. Could we use timezone.utc here, matching the existing state tests?
There was a problem hiding this comment.
Fixed in b4ab6cc. I replaced datetime.UTC with timezone.utc, matching the existing state tests, and ran this integration suite under CPython 3.10.20: 66 passed.
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Could we add one-field mutations for the remaining plan/state joins here? In particular, run/config/plan-reference drift, shard index/partition/workspace, readiness record identities/timestamp, and finalization identities/counts/reference paths are not exercised. Because every condition funnels through _require, statement and branch coverage can stay at 100% even if one of these comparisons regresses.
There was a problem hiding this comment.
Fixed in b4ab6cc. I added explicit one-field mutation coverage for run/config/plan-reference drift; shard identity/index/range/partition/workspace/time; readiness identities/time/deployment metadata; planned-attempt identity and plan references; and finalization identities, ordinals, counts, resume modes, paths, references, lifecycle state, and chronology. The focused suite now has 66 passing tests, and integration.py remains at 100% statement and branch coverage with comparison-specific assertions behind that number.
| _require(actual == expected, "initial readiness deployments do not match the resolved plan") | ||
| return readiness | ||
|
|
||
| def validate_planned_attempt( |
There was a problem hiding this comment.
Could we add a batch-level companion for attempts created by one submission? With the two-shard plan, (shard-00000, attempt-0001, 4101_0) and (shard-00001, attempt-0001, 9999_1) both pass this method, and validate_attempt_set also accepts them because the scheduler identities are unique. That lets one planned Slurm array be represented by unrelated jobs. A pure helper taking the selected planned shards and attempts plus the array_job_id returned by sbatch could require one job ID and the exact shard/task mapping; initial submission can pass all shards and retry can pass its selected subset.
There was a problem hiding this comment.
Looking at this again against the persisted-state boundary, the proposed batch helper would not give us a durable invariant: the plan pins shard-to-task mapping but does not record submission batches, and passing the array_job_id returned by the same sbatch call back into validation would only check the writer against itself. Fresh-process state also cannot reconstruct retry batch membership without another persisted contract. I’m withdrawing this suggestion for #889; any ordinal-1 cohort or submission-group rule belongs with the Stage 2 state/retry design.
There was a problem hiding this comment.
Agreed with your follow-up withdrawal. I did not add a submission-batch helper: without persisted cohort membership it would only validate the writer against itself. Any ordinal-1 cohort or retry submission-group invariant remains deferred to the Stage 2 state/retry design; #889 stays limited to durable plan/state joins.
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Greptile SummaryThe PR adds a reusable validation seam that checks persisted Slurm runtime records against one immutable resolved plan.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/integration.py | Adds reusable plan-aware validators for shards, readiness, attempts, and finalization records; no eligible follow-up defect was established. |
| packages/data-designer-slurm/tests/integration/test_integration_validation.py | Adds comprehensive positive, mutation, ordering, resume-mode, and terminal-state validation coverage. |
| packages/data-designer-slurm/tests/integration/golden/finalization_chain.json | Adds a sanitized complete golden record chain used by integration tests. |
| packages/data-designer-slurm/src/data_designer/slurm/contracts.py | Adds documentation to existing shared path, text, and URL validators without changing behavior. |
| scripts/test_slurm_package_install.py | Extends installed-wheel verification to import the new public PlanStateValidator. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Plan[Resolved Slurm plan] --> Validator[PlanStateValidator]
Run[Run and shard manifests] --> Validator
Readiness[Attempt readiness] --> Validator
Attempt[Attempt manifest] --> Validator
Client[Client result] --> Validator
Candidate[Candidate output] --> Validator
Winner[Shard winner] --> Validator
Validator --> Validated[Validated runtime state]
Reviews (2): Last reviewed commit: "refactor: simplify plan state validator" | Re-trigger Greptile
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
2 participants
📋 Summary
Integrates the reviewed Slurm planning and runtime-state contracts through a pure, plan-aware validation seam. Persisted shards, readiness snapshots, attempts, client results, candidate outputs, and winners are now checked against one resolved plan before downstream runtime or collection code consumes them.
🔗 Related Issue
Closes #880
🔄 Changes
✨ Added
PlanStateValidatorthat computes the resolved-plan reference and shard lookup once per plan.🔧 Changed
PlanStateValidatoras a plain service class with explicit initialization and a read-only plan property.timezone.utcin integration tests so the declared Python 3.10 floor is exercised successfully.🐛 Fixed
CREATEDattempts even when they already carry scheduler metadata.🔍 Attention Areas
integration.py— the plan-scopedPlanStateValidatorservice and its cached contract lookups.test_integration_validation.py— positive, negative, golden, resume-mode, and one-field mutation coverage for all four joins.🧪 Testing
make testpasses — 4,428 passed, 1 skippedmake check-allpassesmake test-slurm-wheel-installpasses✅ Checklist
Description updated with AI